Persistent collections updates (part 7) #182

lorentey · 2022-09-21T17:49:25Z

This fixes the Iterator type (slowing it down by 35% or so, unfortunately), and starts work on PersistentSet. (Including (untested) structural isSubset and isDisjoint tests, but no structural merges yet.)

Checklist

I've read the Contribution Guidelines
My contributions are licensed under the Swift license.
I've followed the coding style of the rest of the project.
I've added tests covering all new code paths my change adds to the project (if appropriate).
I've added benchmarks covering new functionality (if appropriate).
I've verified that my change does not break any existing tests or introduce unexplained benchmark regressions.
I've updated the documentation if necessary.

This slows iteration down by about 37% percent but it now works within ManagedBuffer’s requirements.

msteindorfer · 2022-09-22T09:26:12Z

In general, in case keys are objects, should be consider checking for object identity (i.e., ===) in the persistent collections before calling ==?

Can this be achieved in a single method implementation, or would one need to duplicate the method and add a constraint like Key: AnyObject?

msteindorfer

Change list:

Verify / fix potential issue with semantics of isDisjoint.

Otherwise, LGTM.

Great work on implementing the initial structural operations (i.e., isDisjoint and isSubset) that form the blueprint for structural set-algebra operations, and structural filters, transformations, and (dictionary) merges.

msteindorfer · 2022-09-22T08:41:14Z

Sources/PersistentCollections/Node/_Bitmap.swift

@@ -116,9 +116,14 @@ extension _Bitmap {
 }

 extension _Bitmap {
+  @inlinable @inline(__always)
+  internal func isSubset(of other: Self) -> Bool {
+    _value & ~other._value == 0


Sources/PersistentCollections/Node/_Node+isDisjoint.swift

msteindorfer · 2022-09-22T09:43:10Z

Sources/PersistentCollections/Node/_Node+isDisjoint.swift

+  @inlinable @inline(never)
+  internal func isDisjoint<Value2>(
+    _ level: _Level,
+    with other: _Node<Key, Value2>
+  ) -> Bool {


Consider adding benchmarks for isDisjoint and other structural operations.

Structural operations with persistent data structures have an inherent benefit over performing those operations on a logical level. Hence, it would be great to highlight those differences with benchmarks.

Yep.

Note: Benchmarking these well is a challenge -- the state space is so large, and the behavior is so dependent on the random hash seed, that it's hard to come up with something that truly demonstrates practical benefits.

Sources/PersistentCollections/Node/_Node+isEqual.swift

Sources/PersistentCollections/Node/_Stack.swift

msteindorfer · 2022-09-22T10:24:40Z

Sources/PersistentCollections/Node/_Stack.swift

+#if arch(x86_64) || arch(arm64)
+  @inlinable
+  @inline(__always)
+  internal static var capacity: Int { 13 }
+
+  @usableFromInline
+  internal var _contents: (
+    Element, Element, Element, Element,
+    Element, Element, Element, Element,
+    Element, Element, Element, Element,
+    Element
+  )
+#else
+  @inlinable
+  @inline(__always)
+  internal static var capacity: Int { 7 }
+
+  // xxxxx xxxxx xxxxx xxxxx xxxxx xxxxx xx
+  @usableFromInline
+  internal var _contents: (
+    Element, Element, Element, Element,
+    Element, Element, Element
+  )
+#endif


Is hashValue in Swift on 32-bit systems 32 bits as well?

Yep! Hasher returns exactly Int-sized globs of bits.

It's a shame we have to hardwire these lengths here, by the way, and also that we have to manually list architectures instead of just directly asking the compiler about the size of Int...

msteindorfer · 2022-09-22T10:26:00Z

Sources/PersistentCollections/PersistentDictionary/PersistentDictionary+Filter.swift

-      // However, this requires a series of `UInt32._bit(ranked:)` invocations
-      // that may or may not be meaningfully better than simply rehashing the
-      // key.
+      // FIXME: We could do this as a structural transformation.


Consider that structural formations must deal with compaction in order to guarantee a canonical representation, which slightly complicates the matter.

msteindorfer · 2022-09-22T10:32:40Z

Sources/PersistentCollections/PersistentDictionary/PersistentDictionary+MapValues.swift

@@ -22,17 +22,13 @@ extension PersistentDictionary {
  public func compactMapValues<T>(
    _ transform: (Value) throws -> T?
  ) rethrows -> PersistentDictionary<Key, T> {
+    // FIXME: We could do this as a structural transformation.


Consider that structural formations must deal with compaction in order to guarantee a canonical representation, which slightly complicates the matter.

It certainly does. Trying to do this in place (for Set.formIntersection and the like) would probably be a fool's errand, but I think an algorithm that constructs a new instance should be reasonably doable! (This is next on my list for part 9, actually. 🤓)

msteindorfer · 2022-09-22T10:33:25Z

Sources/PersistentCollections/PersistentDictionary/PersistentDictionary+Merge.swift

@@ -15,6 +15,7 @@ extension PersistentDictionary {
    _ keysAndValues: __owned S,
    uniquingKeysWith combine: (Value, Value) throws -> Value
  ) rethrows where S.Element == (Key, Value) {
+    // FIXME: Do a structural merge


lorentey · 2022-09-22T18:06:30Z

There isn't really a good way to change behavior based on properties of a generic type argument (other than using the type constraints required in the declaration)

Doing things like foo is AnyObject can backfire pretty badly if the check survives into the generated code (e.g. when foo is of a type that bridges with Objective C).

The === shortcut is best implemented within the Equatable.== implementation of the Key type, like we do for the collection types themselves.

Co-authored-by: Michael Steindorfer <michael@steindorfer.name>

msteindorfer

LGTM

Co-authored-by: Michael Steindorfer <michael@steindorfer.name>

…mparisons; add a fast path for collision nodes

lorentey · 2022-09-22T23:35:31Z

@swift-ci test

msteindorfer · 2022-09-23T16:23:22Z

Sources/PersistentCollections/Node/_Node+isEqual.swift

+  internal func isEqual<Value2>(
+    to other: _Node<Key, Value2>,
+    by areEquivalent: (Value, Value2) -> Bool
  ) -> Bool {


Why is it necessary to introduce the additional generic Value2 here?

This isn't exercised, and there is a chance it won't be, in which case we can remove it later. (But it doesn't really have a performance drawback, so it's no big deal if it stays in.)

The reason I wanted to leave this door open is in case we want to tackle a weird asymmetry in SetAlgebra. It is customary for SetAlgebra types to provide flexible overloads for isSubset/isDisjoint/intersection/etc that can take more types than just Self -- elsewhere I usually have an unconstrained generic overload that can take any Sequence plus a handful of concrete overloads for select types for which I can do a more efficient implementation. (So e.g. OrderedSet provides not only isSubset(of other: Self), but also isSubset(of other: Set<Element>) and isSubset(of other: some Sequence<Element>).) The asymmetry I find weird is that this applies to all such operations except Equatable.== -- why can't I easily check if a Set and an OrderedSet contain exactly the same members?

In the case of PersistentSet, we currently have isSubset(of:) defined for Self and PersistentDictionary<Element, T>.Keys arguments. (Later on we will also need one that can take any SetAlgebra and one that can take any Sequence.) If we do end up adding an isEqual(to:) family of methods that generalize ==, then we will need to this particular flavor of generic definition in _Node to be able to compare a persistent set to the Keys view of any persistent dictionary that has a matching Key type.

(I wrote this up (well, copy-pasted from above) as #183.)

lorentey · 2022-09-23T18:17:18Z

@swift-ci test

lorentey and others added 4 commits September 21, 2022 10:45

[Utilities] Adjust Swift 5.7 version check

e19ba8e

[PersistentDictionary] Small cleanups

c355812

[PersistentCollections] Fix memory management for the hash tree iterator

04b134e

This slows iteration down by about 37% percent but it now works within ManagedBuffer’s requirements.

[PersistentSet] Start drafting the new type

958fd47

lorentey requested a review from msteindorfer September 21, 2022 17:49

msteindorfer requested changes Sep 22, 2022

View reviewed changes

Update Sources/PersistentCollections/Node/_Node+isDisjoint.swift

24619df

Co-authored-by: Michael Steindorfer <michael@steindorfer.name>

msteindorfer approved these changes Sep 22, 2022

View reviewed changes

lorentey and others added 3 commits September 22, 2022 16:31

Update Sources/PersistentCollections/Node/_Stack.swift

f2539a4

Co-authored-by: Michael Steindorfer <michael@steindorfer.name>

[PersistentCollections] _Node.isDisjoint: Add new fast path

f79fd63

[PersistentCollections] _Node.isEqual: Don’t allow customizing key co…

de13650

…mparisons; add a fast path for collision nodes

msteindorfer reviewed Sep 23, 2022

View reviewed changes

lorentey added 3 commits September 23, 2022 11:15

[Benchmarks] Fix header name in #include

94d979c

Fix dependency declarations in package manifest

1184191

[BitSet.Counted] Fix invariant checking (d’oh)

59858a7

lorentey merged commit b1a45f0 into apple:release/1.1 Sep 23, 2022

lorentey added this to the 1.1.0 milestone Oct 10, 2022

lorentey added the HashTreeCollections Hash-array mapped prefix tree implementations label Nov 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persistent collections updates (part 7) #182

Persistent collections updates (part 7) #182

lorentey commented Sep 21, 2022

msteindorfer commented Sep 22, 2022

msteindorfer left a comment

msteindorfer Sep 22, 2022

msteindorfer Sep 22, 2022

lorentey Sep 22, 2022

msteindorfer Sep 22, 2022

lorentey Sep 22, 2022

lorentey Sep 22, 2022

msteindorfer Sep 22, 2022

msteindorfer Sep 22, 2022

msteindorfer Sep 22, 2022

msteindorfer Sep 22, 2022

lorentey Sep 22, 2022

msteindorfer Sep 22, 2022

lorentey commented Sep 22, 2022

msteindorfer left a comment

lorentey commented Sep 22, 2022

msteindorfer Sep 23, 2022

lorentey Sep 23, 2022

lorentey Sep 23, 2022

lorentey commented Sep 23, 2022

Persistent collections updates (part 7) #182

Persistent collections updates (part 7) #182

Conversation

lorentey commented Sep 21, 2022

Checklist

msteindorfer commented Sep 22, 2022

msteindorfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorentey commented Sep 22, 2022

msteindorfer left a comment

Choose a reason for hiding this comment

lorentey commented Sep 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorentey commented Sep 23, 2022